Skip to content

Replace UAF death test with deterministic stale-delegate assertion - #58321

Closed
fkgozali wants to merge 1 commit into
react:mainfrom
fkgozali:export-D118205636
Closed

Replace UAF death test with deterministic stale-delegate assertion#58321
fkgozali wants to merge 1 commit into
react:mainfrom
fkgozali:export-D118205636

Conversation

@fkgozali

@fkgozali fkgozali commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary:
SchedulerDelegateInvalidationTest.DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAF
asserted a real use-after-free through EXPECT_DEATH: it destroyed the
RecordingDelegate, then drained pendingRenderingUpdates_ so the queued lambda
dereferenced the freed object, and expected the process to die. Undefined behaviour
is not a reliable process-termination signal, without a sanitizer the freed read
can simply succeed, and gtest then reports Result: failed to die. The death test
also has to fork() a multi-threaded process.

This replaces the death test with a deterministic assertion on the same property.
Instead of destroying the delegate, the test detaches it via
Scheduler::setDelegate(nullptr) and keeps it alive, then drains the pending
rendering update and asserts that the drained lambda still invokes
schedulerShouldRenderTransactions on the detached delegate.

That pins exactly the coverage the death test was after: Scheduler::setDelegate is
a plain assignment, so a lambda already queued by uiManagerDidFinishTransaction
keeps the raw delegate pointer it captured, and draining it after the delegate has
been detached still calls through that pointer, which is a use-after-free when the
delegate has been destroyed rather than merely detached. Same property, no undefined
behaviour and no fork(). It matches the shape of the existing
UnregisterSurface_DoesNotDrainPendingRenderingUpdates test in the same file.

The underlying window is unchanged: nothing in Scheduler::setDelegate cancels
rendering updates that are already queued, so closing it needs a shutdown signal at
the runtime-scheduler level. That is a design decision for the owners rather than a
test fix.

Changelog: [Internal]

Differential Revision: D118205636

Summary:
`SchedulerDelegateInvalidationTest.DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAF`
asserted a real use-after-free through `EXPECT_DEATH`: it destroyed the
`RecordingDelegate`, then drained `pendingRenderingUpdates_` so the queued lambda
dereferenced the freed object, and expected the process to die. Undefined behaviour
is not a reliable process-termination signal, without a sanitizer the freed read
can simply succeed, and gtest then reports `Result: failed to die.` The death test
also has to `fork()` a multi-threaded process.

This replaces the death test with a deterministic assertion on the same property.
Instead of destroying the delegate, the test detaches it via
`Scheduler::setDelegate(nullptr)` and keeps it alive, then drains the pending
rendering update and asserts that the drained lambda still invokes
`schedulerShouldRenderTransactions` on the detached delegate.

That pins exactly the coverage the death test was after: `Scheduler::setDelegate` is
a plain assignment, so a lambda already queued by `uiManagerDidFinishTransaction`
keeps the raw delegate pointer it captured, and draining it after the delegate has
been detached still calls through that pointer, which is a use-after-free when the
delegate has been destroyed rather than merely detached. Same property, no undefined
behaviour and no `fork()`. It matches the shape of the existing
`UnregisterSurface_DoesNotDrainPendingRenderingUpdates` test in the same file.

The underlying window is unchanged: nothing in `Scheduler::setDelegate` cancels
rendering updates that are already queued, so closing it needs a shutdown signal at
the runtime-scheduler level. That is a design decision for the owners rather than a
test fix.

Changelog: [Internal]

Differential Revision: D118205636
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 3, 2026
@meta-codesync

meta-codesync Bot commented Sep 3, 2026

Copy link
Copy Markdown

@fkgozali has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118205636.

@cortinico cortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

@facebook-github-tools facebook-github-tools Bot added p: Facebook Partner: Facebook Partner labels Sep 3, 2026
@meta-codesync meta-codesync Bot closed this in d9ad3f0 Sep 3, 2026
@meta-codesync meta-codesync Bot added the Merged This PR has been merged. label Sep 3, 2026
@meta-codesync

meta-codesync Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request has been merged in d9ad3f0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. meta-exported p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants